test: native_reclaim_cycles could not reach the defect it guards (#1138) - #1193

Adversarial review of 795943b, built and run in the audit container on PG 18.4 (assert build).

Your four cells reproduce here exactly, including the free list of 18. Then I tried to isolate why the new suite sees what the old one missed, and the answer is not the one the comment gives.

First, the reproduction

Same three cells, my own mutation of the #84 fix (the CommandCounterIncrement() at src/columnar_metadata.c:930, removed whole with its comment, restored afterwards, tree clean):

new suite / clean build      14 passed + 0 failed      free_space rows: 18
new suite / MUTATED build     9 passed + 5 FAILED      all five: tuple already updated by self
old suite / MUTATED build    12 passed + 0 failed      <- the defect, same .so

The sharpest way to put the defect: the old suite already had the arm named after the bug — compact_rewrite cycle N returns a count (no self-conflict) — and it passed on a binary with the fix deleted. A missing arm is a gap; an arm that cannot reach its condition is worse, because it reads as coverage.

Worth noting from the mutated run: every parity after compact_rewrite cycle N arm still PASSED while the rewrite was erroring. The statement aborts, the data does not change, and parity holds. So the parity arms can never detect this class of defect, and the count arm is carrying the whole guard.

The finding: the fixture change is not what makes this work

The comment attributes the old blindness to free-list fragmentation:

One row is not two allocations, so the just-consumed row was never re-selected and the missing CommandCounterIncrement cost nothing observable.

I could not make that hold. reclaim_coalesce=off changes two things at once, and the second one is not mentioned: insert_free_space_row carries its own CommandCounterIncrement() at columnar_metadata.c:785 and :792, both guarded by if (pgcolumnar_reclaim_coalesce). With coalescing on, that CCI makes the consumption visible and masks the missing one at :930.

So I separated the two factors. Fragmentation does not have to come from the GUC: freeing alternate whole groups leaves ranges that are not adjacent, so coalescing has nothing to merge and the free list fragments with the setting left at its default. That gives an identical fixture under both settings. All four cells on the same mutated binary:

Read the corners. Fifteen fragmented ranges with coalescing on does not reach the defect. One single range with coalescing off does. reclaim_coalesce=off is necessary and sufficient here; fragmentation is neither.

Three consequences:

1. The comment is wrong about the mechanism, and it is the load-bearing kind of wrong: it tells the next maintainer that the fixture's fragmentation is what must be preserved. It is not. Someone tuning ROWS, DEL_LO/DEL_HI or the group size to keep the free list large would be protecting the wrong property.

2. premise: the free list is fragmented, so one command allocates from it more than once does not gate what it says. My bottom-right cell catches the defect with a free list of one. The premise is not wrong to exist — a fixture that stopped fragmenting is worth knowing about — but its stated justification is not what makes the arm below it work, and a floor of 5 against a measured 18 pins neither.

3. The guard only covers the non-default configuration, and that is the part I would not leave unsaid. pgcolumnar_reclaim_coalesce = true at columnar_metadata.c:682 is the shipped default. My top-left cell says that under that default, the #84 fix can be deleted and nothing in this suite — or, on this evidence, anywhere — notices. That is not an argument against your change: :930 is exactly the CCI the coalesce path does not provide, so the fix is right and testing it with coalescing off is the correct way to reach it. It is an argument for saying so out loud, because right now the suite reads as "compact_rewrite does not self-conflict" when what it pins is "compact_rewrite does not self-conflict with coalescing off".

What I would change: keep the suite exactly as it runs, and rewrite the comment to say that reclaim_coalesce=off is what makes the defect reachable, because coalescing's own CCI at :785/:792 masks the missing one — with the four cells above as the evidence if you want them. Then decide separately whether the default configuration deserves its own guard, which is a new issue rather than this PR's job.

Smaller things

PGC_EXTRA_CONF fails closed, which is the right shape. It is a real lib.sh facility (lib.sh:374), and the composition preserves a caller's existing value. If the GUC name were ever misspelled, PostgreSQL would accept it as a placeholder and silently do nothing — but the fragmentation premise below would then fail, so a typo surfaces as a red premise rather than a quiet loss of coverage. Worth the sentence it does not currently have.

Both premise arms fail closed on an empty read. ${_groups:-0} and ${_free:-0} turn a failed query into TOO FEW () against an expected many (), which is a mismatch. I checked because the idiom builds the expected value out of the measured one, which is usually how a tautological arm is born; this one is not.

Harness independence holds. The only mention of the pytest twin is in CHANGELOG.md, which is where the project's rule permits it. No import, no invocation, no reference across the boundary.

The coalescing-in-cluster-config point is correct and I verified the reason. Every psql_run is its own session, so a SET would not reach the writing session. Your note that my pytest twin does not have this problem because it holds one connection is right, and it is a real structural difference between the harnesses rather than a porting detail.

Not approving yet: mergeStateStatus=UNSTABLE, 2 checks still pending. The suite is a genuine improvement and I want it in; it is the explanation attached to it that I am asking you to change.

Container pgcolumnar-audit, PG 18.4 assert build. Mutation applied and restored for every cell, git status clean at the end, and the #84 CCI asserted absent from the source under test in each run.

The diagnosis matches what is already on main in the pytest twin (test_native_reclaim_cycles.py already sets reclaim_coalesce = off). Bringing the shell suite onto the same fixture, asserting the GUC read-back, and putting the setting in PGC_EXTRA_CONF rather than a one-shot SET are the right moves. Approving.

Approving c0a0c3d. 15 checks, all SUCCESS, CLEAN.

I verified the fix on this head rather than taking the numbers on report.

The pair grades one-for-one, and the twin now asserts the property that arms it:

$ python3 compare_to_bash.py ../native_reclaim_cycles.sh test_native_reclaim_cycles.py
PROPERTIES IN THE BASH SUITE AND NOT IN THE PORT:
  none -- every bash property is asserted by name in the port
literal matches: 4 | template matches: 2 | missing: 0
VERDICT: every bash property is covered

$ pytest --pgc-expect-tests 398 <the guard half>
398 passed, checks run: 1100, accounting: 1100 pass + 0 fail + 0 unrun

test_native_reclaim_cycles.py:119 now reads SHOW pgcolumnar.reclaim_coalesce and asserts off, where before it set the GUC at line 80 and never checked it took. The docstring at lines 24-26 now names the two things the GUC does and the if (pgcolumnar_reclaim_coalesce) guard on the increment, which is the account the four cells support.

Your removal proof is the right one and it answers the objection I would have raised. missing: 0 is a statement about text: it proves the name appears in the port, not that anything records under it. Deleting the SET and nothing else, and getting

premise: coalescing is off, which is what lets this suite reach #84: got 'on' want 'off'

is what turns the name into a check. Asserting the mutant still parses, restoring the source byte-identical and clearing __pycache__ on both sides are the three things that make that proof mean what it says.

On the parity arm. It caught a divergence in meaning, not in spelling: the shell side had been corrected and the port was still teaching the mechanism the measurements disproved, with a docstring a maintainer would have believed. A guard that only compared counts would have graded 0 and said nothing. That is worth remembering the next time #1046's two-directional question comes up, because the value here came from the direction that was already implemented.

One thing I like about the shape this landed in. The free-list count is printed and not asserted, and the GUC is asserted and not assumed. That is the right way round: the fixture's shape is context for a reader, and the setting is the precondition without which the suite cannot fail. It reads correctly now to someone who has never seen this thread.

Merging remains @jdatcmd's call. With this one in, the only open PRs are linuxhikerpm's three, and #1155's ceilings-not-bands is the one I would want addressed before any of those land.